-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FIX] remove extra system headers on direct gets #4618
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the performance impact on direct get performance due to these?
Should we make one function that removes them all at once?
We would have to scan at least once, but a better question is perhaps we should be doing this on the on-board? The issue is republishing is causing a recapture by the stream. If the stream is guaranteed not to have the header, then no cleanup would be necessary. |
2916bb0
to
36b4e2d
Compare
@derekcollison discussed with @neilalexander an alternative strategy - remove these headers before onboarding into the stream. That keeps direct as fast as possible and prevents serialization of any |
(and yes, I added one function to remove all in one shot) |
Made a change to constrain the actual headers that are removed as some are handled further down during the Store() operation. |
@@ -4399,6 +4400,11 @@ func (mset *stream) processJetStreamMsg(subject, reply string, hdr, msg []byte, | |||
} | |||
} | |||
|
|||
// if we are NOT doing subject remapping, this is a message to on-board it shouldn't store Nats-* headers | |||
if tsubj == _EMPTY_ { | |||
hdr = removeStreamIdentityHeaders(hdr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More importantly - this doesn't fix the issue for direct get - it simply solves it for the on-boarding. We would still need to have the initial fix on the outbound to ensure that clients won't get duplicate headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes since they are stored with headers today..
… it simply appends JSON bytes to existing headers. If the message was onboarded on a republish, this will contain system headers. Since headers are not ordered and can contain multiple values for the same header, this can break KV clients as well as create ambiguity on the stream that yielded the message. Fix #4573
@derekcollison this is possibly the correct fix, if we think that we want to reduce the surface area, we can keep the on-boarding of the the stream identity headers, but this is the best we can do. The direct API checks if there's a |
I am conflicted, I think we need to retain what the app sent us. |
[FIX] direct get APIs can contain duplicate
Nats-Stream
,Nats-Sequence
,Nats-Time-Stamp
andNats-Subject
headers because it simply appends JSON bytes to existing headers. If the message was onboarded on a republish, this will contain the above system headers. Since headers are not ordered and can contain multiple values for the same header, this can break KV clients and create ambiguity on the stream that yielded the message.Resolves #NNN
git pull --rebase origin main
)Resolves #4573
Changes proposed in this pull request:
Nats-Stream
,Nats-Sequence
,Nats-Time-Stamp
andNats-Subject
that may be present on the stream message when using direct get apis